-
Notifications
You must be signed in to change notification settings - Fork 298
Implement cloud configuration support for Azure SDK for Rust #2898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: RickWinter <[email protected]>
…ensive tests Co-authored-by: RickWinter <[email protected]>
Co-authored-by: RickWinter <[email protected]>
pub fn new_with_options( | ||
account_name: &str, | ||
credential: Arc<dyn azure_core::credentials::TokenCredential>, | ||
options: ClientOptions, | ||
) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be new(......).
The options param is optional and should be:
options: Option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to preface comments to copilot with @copilot
. Also, this is an example, though I'm not sure an example of what.
pub fn new_for_public_cloud( | ||
account_name: &str, | ||
credential: Arc<dyn azure_core::credentials::TokenCredential>, | ||
) -> Self { | ||
let options = ClientOptions::default() | ||
.with_cloud_config(configurations::azure_public_cloud()) | ||
.with_audience("https://storage.azure.com"); | ||
|
||
Self::new_with_options(account_name, credential, options) | ||
} | ||
|
||
/// Create a new Tables client for Azure China Cloud. | ||
pub fn new_for_china_cloud( | ||
account_name: &str, | ||
credential: Arc<dyn azure_core::credentials::TokenCredential>, | ||
) -> Self { | ||
let options = ClientOptions::default() | ||
.with_cloud_config(configurations::azure_china_cloud()) | ||
.with_audience("https://storage.azure.com"); | ||
|
||
Self::new_with_options(account_name, credential, options) | ||
} | ||
|
||
/// Create a new Tables client for Azure US Government Cloud. | ||
pub fn new_for_us_government_cloud( | ||
account_name: &str, | ||
credential: Arc<dyn azure_core::credentials::TokenCredential>, | ||
) -> Self { | ||
let options = ClientOptions::default() | ||
.with_cloud_config(configurations::azure_us_government_cloud()) | ||
.with_audience("https://storage.azure.com"); | ||
|
||
Self::new_with_options(account_name, credential, options) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do NOT add methods for each cloud. The cloud configuration is only used if passed in via the options bag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of this doesn't match our guidelines. We need to consider also how users will use it, not just go off and make changes based on some loose descrpiption. This is why I wanted to add more to the bug after our discussion before anyone - or anything - worked on it. There wasn't enough there at the time this PR was started.
@copilot in addition to requested changes, also review the original bug #2439 for updated comments.
sdk/core/azure_core/src/cloud.rs
Outdated
pub fn get_service_audience(&self, service: &str) -> Option<&str> { | ||
self.service_audiences.get(service).map(|s| s.as_str()) | ||
} | ||
|
||
/// Gets the audience for Azure Resource Manager. | ||
pub fn get_resource_manager_audience(&self) -> &str { | ||
&self.resource_manager_audience | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot per guidelines, get accessors must not have a get_
prefix. Remove get_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the get_
prefix from accessor methods. The methods are now service_audience()
and resource_manager_audience()
. Commit: 591339a
sdk/core/azure_core/src/cloud.rs
Outdated
) | ||
.with_service_audience("storage", "https://storage.azure.com") | ||
.with_service_audience("keyvault", "https://vault.azure.net") | ||
.with_service_audience("tables", "https://storage.azure.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove this line. We do not yet support Storage tables. Remove all similar lines below for tables along with tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all tables service references from cloud configurations and tests since Storage tables is not yet supported. Commit: 591339a
sdk/core/azure_core/src/constants.rs
Outdated
/// These constants are deprecated. Use [`crate::cloud::configurations`] instead | ||
/// for a more comprehensive cloud configuration system that includes authority hosts, | ||
/// resource manager endpoints, and service audiences. | ||
#[deprecated(since = "0.29.0", note = "Use `azure_core::cloud::configurations` instead")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this should be 0.28.0, as well as below. We've not yet published 0.28.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed version numbers from 0.29.0 to 0.28.0 in constants.rs deprecation notices. Commit: 591339a
pub fn new_with_options( | ||
account_name: &str, | ||
credential: Arc<dyn azure_core::credentials::TokenCredential>, | ||
options: ClientOptions, | ||
) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to preface comments to copilot with @copilot
. Also, this is an example, though I'm not sure an example of what.
docs/CLOUD_CONFIGURATION.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove this file. We don't document features like this. Put enough information into doc comments on the Rust source with ///
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed CLOUD_CONFIGURATION.md file. Added comprehensive documentation examples in the Rust source using ///
comments instead. Commit: 591339a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove this file. It shows nothing useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the cloud_configuration.rs example file from azure_identity. Commit: 591339a
@@ -20,6 +21,7 @@ pub struct TokenCredentialOptions { | |||
pub(crate) env: Env, | |||
pub(crate) http_client: Arc<dyn HttpClient>, | |||
pub(crate) authority_host: String, | |||
pub(crate) cloud_config: Option<&'static CloudConfiguration>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this should be called only cloud
of type Option<CloudConfiguration>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed field to cloud
with type Option<CloudConfiguration>
as specified. Commit: 591339a
pub fn new_for_public_cloud() -> Self { | ||
let mut options = Self::default(); | ||
options.set_cloud_config(azure_core::cloud::configurations::azure_public_cloud()); | ||
options | ||
} | ||
|
||
/// Create options for the Azure China Cloud. | ||
pub fn new_for_china_cloud() -> Self { | ||
let mut options = Self::default(); | ||
options.set_cloud_config(azure_core::cloud::configurations::azure_china_cloud()); | ||
options | ||
} | ||
|
||
/// Create options for the Azure Germany Cloud. | ||
pub fn new_for_germany_cloud() -> Self { | ||
let mut options = Self::default(); | ||
options.set_cloud_config(azure_core::cloud::configurations::azure_germany_cloud()); | ||
options | ||
} | ||
|
||
/// Create options for the Azure US Government Cloud. | ||
pub fn new_for_us_government_cloud() -> Self { | ||
let mut options = Self::default(); | ||
options.set_cloud_config(azure_core::cloud::configurations::azure_us_government_cloud()); | ||
options | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove these constructors. We don't have constructors on client options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed all constructor functions from TokenCredentialOptions
. Configuration is now done via the builder pattern with with_cloud()
. Commit: 591339a
} | ||
|
||
/// Gets the cloud configuration if set. | ||
pub fn cloud_config(&self) -> Option<&'static CloudConfiguration> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove this. We don't get options this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the cloud_config()
getter method. Users should configure clouds via builder methods instead. Commit: 591339a
/// Set the cloud configuration for authentication requests. | ||
/// | ||
/// This allows credentials to work with different Azure clouds | ||
/// (Public, China, Germany, US Government) by setting the appropriate | ||
/// authority host and other cloud-specific settings. | ||
pub fn set_cloud_config(&mut self, cloud_config: &'static CloudConfiguration) { | ||
self.cloud_config = Some(cloud_config); | ||
self.authority_host = cloud_config.authority_host.to_string(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove this. We don't set configuration this way. @chlowell we also need to evaluate why there's a set_authority_host
and make this work more like our guidelines dictate for client options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the set_cloud_config()
method. Configuration is now done via the builder pattern with with_cloud()
. Commit: 591339a
…lines Co-authored-by: heaths <[email protected]>
/// // Configure for Azure China Cloud | ||
/// let options = ClientOptions::default() | ||
/// .with_cloud(configurations::azure_china_cloud().clone()) | ||
/// .with_audience("https://storage.azure.com"); | ||
/// | ||
/// // Get the OAuth scope for authentication | ||
/// let scope = options.get_auth_scope(Some("storage")); | ||
/// assert_eq!(scope, Some("https://storage.azure.com/.default".to_string())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RickWinter @LarryOsterman @chlowell what do we think of this? Seems okay, but the DX for Identity kinda stinks currently, but that's less the cloud design and more about options overall.
This PR implements comprehensive cloud configuration support for the Azure SDK for Rust, enabling services to work seamlessly across different Azure cloud environments including Azure Public Cloud, Azure China Cloud, Azure Germany Cloud, and Azure US Government Cloud.
Problem
Previously, the Azure SDK for Rust only supported Azure Public Cloud through hardcoded constants in
authority_hosts
andresource_manager_endpoint
modules. Using the SDK with sovereign clouds required workarounds like customTokenCredential
implementations, creating friction for developers working in Azure China, Germany, or US Government clouds.Solution
This implementation provides a comprehensive cloud configuration system that:
🏗️ Core Infrastructure
azure_core::cloud
module withCloudConfiguration
struct containing authority hosts, resource manager endpoints, and service-specific audiencesconfigurations::azure_public_cloud()
, etc.CloudConfiguration::audience_to_scope()
converts audience URIs to OAuth scopes🔧 Enhanced Client Options
cloud
field of typeOption<CloudConfiguration>
toClientOptions
with_cloud()
for easy configurationget_auth_scope()
method automatically derives correct OAuth scopes based on cloud and service🔐 Identity Integration
TokenCredentialOptions
with cloud configuration support viawith_cloud()
methodUsage Examples
Configuring client options for specific clouds:
Credential configuration:
API Design
The implementation follows Azure SDK guidelines:
get_
prefixes on accessor methods (service_audience()
,resource_manager_audience()
)Option<T>
typesBackward Compatibility
The old
authority_hosts
andresource_manager_endpoint
modules are deprecated but remain functional, ensuring existing code continues to work while encouraging migration to the new cloud configuration system.Benefits
Testing
Added comprehensive tests covering all new functionality including cloud configuration creation, scope derivation, credential integration, and backward compatibility. All existing tests continue to pass.
Fixes #2439.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.